-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-128627: Emscripten: Use wasm-gc based call adaptor if available #128628
Conversation
Part of the ongoing quest to support JSPI. The JSPI spec removed its dependence on JS type reflection, and now the plan is for runtimes to ship JSPI while keeping type reflection in stage 3. So we need an alternative way to count the number of parameters of a function. It is possible to count them by repeatedly trying to instantiate a webassembly module with the function as an import of a different type signature. But this is pretty inefficient. Since WebAssembly gc is now stage 4, there is a new option. WebAssembly gc added the `ref.test` instruction which can ask if a funcref has a given type. It's a bit difficult to apply because even our usual assembler the wasm binary toolkit doesn't support this instruction yet. But all JS engines that support JSPI support it. We just have to do some manual work to produce the binary. This code also has to be written carefully to interact properly with memory snapshots. Importantly, no JS initialization code can be called from the C initialization code. For this reason, we make a C function pointer to fill from JS and fill it in a preRun function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit I'm a little hazy on the specifics here; my biggest concern is your comment on the original ticket about toolchain support for wasm-gc being poor. Is that the reason for the "big blob of WASM"? i.e., that compilers can't (currently) generate the required WASM yet, so we have to hard-code the WASM to access the wasm-gc feature that is needed?
Assuming that's the case, I think this all makes sense - and at the very least, it's isolated to the Emscripten build (with the one exception in the pyruntimestate).
I've flagged a couple of other minor issues; plus this PR needs a NEWS entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems to be using 2-space indenting rather than 4-space for all other C code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've marked this resolved... but it looks like you've gone the opposite direction to what I was intending/suggesting. Python's C code is 4-space indented; the updates you've made here move everything to 2-space. It should be 4-space for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry I misread your comment. It's hard to remember all these different code styles.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Yeah exactly. Even the assembler I usually use can't produce this instruction. Ideally, there would be a webassembly clang intrinsic for checking if a function pointer has the right signature and then we could write this in C like: if (__builtin_wasm_call_signature_matches((three_arg)func)) {
return ((three_arg)func)(self, args, kw);
}
if (__builtin_wasm_call_signature_matches((two_arg)func)) {
return ((two_arg)func)(self, args);
}
// ...
PyErr_SetString(PyExc_SystemError,
"Handler has unexpected call signature"); and then table.get $func_ptr_table $func
ref.test $three_arg_type instructions. Though to be honest, I don't really know how |
So, at the moment there's no support for wasm gc in clang. It's correct what @hoodmane is saying. With wasmgc support in the toolchain the blob of wasm wouldn't be necessary anymore. However, while working on this the client that was paying us had a change of priorities and we couldn't finish this work. It's very unfortunate. If there's anyone wishing to fund the completion of this work, please let me know or contact Igalia directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good, except for the 4 vs 2 indent issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've marked this resolved... but it looks like you've gone the opposite direction to what I was intending/suggesting. Python's C code is 4-space indented; the updates you've made here move everything to 2-space. It should be 4-space for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for those fixes!
I'm not sure what's going on with the Hypothesis glob test failure - I can't see any reason this would be related to this change specifically. Hypothesis tests sometimes pass on a repeated run, but this has now failed on 2 repeats. |
Well now CI looks green. All changes are behind |
Yeah - third time was the charm :-) That particular test turns out to be a known intermittent problem - #109959. |
Thanks @freakboy3742! |
I figured out a way to remove the super inefficient `calculate_wasm_func_nargs_fallback` and replace it with wasm-gc code. This should perform much better. The cpython patch message says: Part of the ongoing quest to support JSPI. The JSPI spec removed its dependence on JS type reflection, and now the plan is for runtimes to ship JSPI while keeping type reflection in stage 3. So we need an alternative way to count the number of parameters of a function. It is possible to count them by repeatedly trying to instantiate a webassembly module with the function as an import of a different type signature. But this is pretty inefficient. Since WebAssembly gc is now stage 4, there is a new option. WebAssembly gc added the `ref.test` instruction which can ask if a funcref has a given type. It's a bit difficult to apply because even our usual assembler the wasm binary toolkit doesn't support this instruction yet. But all JS engines that support JSPI support it. We just have to do some manual work to produce the binary. This code also has to be written carefully to interact properly with memory snapshots. Importantly, no JS initialization code can be called from the C initialization code. For this reason, we make a C function pointer to fill from JS and fill it in a preRun function. Upstream PR: python/cpython#128628
I figured out a way to remove the super inefficient `calculate_wasm_func_nargs_fallback` and replace it with wasm-gc code. This should perform much better. The cpython patch message says: Part of the ongoing quest to support JSPI. The JSPI spec removed its dependence on JS type reflection, and now the plan is for runtimes to ship JSPI while keeping type reflection in stage 3. So we need an alternative way to count the number of parameters of a function. It is possible to count them by repeatedly trying to instantiate a webassembly module with the function as an import of a different type signature. But this is pretty inefficient. Since WebAssembly gc is now stage 4, there is a new option. WebAssembly gc added the `ref.test` instruction which can ask if a funcref has a given type. It's a bit difficult to apply because even our usual assembler the wasm binary toolkit doesn't support this instruction yet. But all JS engines that support JSPI support it. We just have to do some manual work to produce the binary. This code also has to be written carefully to interact properly with memory snapshots. Importantly, no JS initialization code can be called from the C initialization code. For this reason, we make a C function pointer to fill from JS and fill it in a preRun function. Upstream PR: python/cpython#128628
Part of the ongoing quest to support JSPI. The JSPI spec removed its dependence on JS type reflection, and now the plan is for runtimes to ship JSPI while keeping type reflection in stage 3. So we need an alternative way to count the number of parameters of a function. It is possible to count them by repeatedly trying to instantiate a webassembly module with the function as an import of a different type signature. But this is pretty inefficient.
Since WebAssembly gc is now stage 4, there is a new option. WebAssembly gc added the
ref.test
instruction which can ask if a funcref has a given type. It's a bit difficult to apply because even our usual assembler the wasm binary toolkit doesn't support this instruction yet. But all JS engines that support JSPI support it. We just have to do some manual work to produce the binary.This code also has to be written carefully to interact properly with memory snapshots. Importantly, no JS initialization code can be called from the C initialization code. For this reason, we make a C function pointer to fill from JS and fill it in a preRun function.